Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Fix range of single commit generation in init #8095

Merged
merged 7 commits into from
Jan 31, 2023
Merged

Fix range of single commit generation in init #8095

merged 7 commits into from
Jan 31, 2023

Conversation

shuse2
Copy link
Collaborator

@shuse2 shuse2 commented Jan 27, 2023

What was the problem?

This PR resolves #8086

How was it solved?

  • Fix the range of single commit generation on node initialization.
  • Refactor the single commit generation loop, to reduce double check of BFT params on the last height

Problem was finalizedBlockHeader.aggregateCommit.height can be removed from the BFT params, and it fails on the initialization. Because maxHeightCertified > finalizedBlockHeader.aggregateCommit.height and up to maxHeightCertified there is no need of single commit. Therefore, we can use maxHeightCertified for the lower range

How was it tested?

  • Updated unit test

@shuse2 shuse2 self-assigned this Jan 27, 2023
@codecov
Copy link

codecov bot commented Jan 27, 2023 •
edited
Loading

Codecov Report

Merging #8095 (6e3c4ff) into development (7366373) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           development    #8095   +/-   ##
============================================
  Coverage        82.64%   82.65%           
============================================
  Files              579      579           
  Lines            21011    21004    -7     
  Branches          3058     3058           
============================================
- Hits             17364    17360    -4     
+ Misses            3647     3644    -3
Impacted Files Coverage Δ
framework/src/engine/consensus/consensus.ts 80.25% <ø> (+0.62%) ⬆️
framework/src/engine/generator/generator.ts 82.20% <100.00%> (-0.25%) ⬇️

@shuse2 shuse2 requested a review from ishantiw January 31, 2023 08:05
janhack
janhack suggested changes Jan 31, 2023
View reviewed changes
Copy link

@janhack janhack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment for one issue.

Otherwise, the PR looks good and I just replied to the discussions.

framework/src/engine/consensus/consensus.ts Show resolved Hide resolved
framework/src/engine/generator/generator.ts Show resolved Hide resolved
framework/src/engine/generator/generator.ts Show resolved Hide resolved
framework/src/engine/generator/generator.ts Show resolved Hide resolved
@shuse2 shuse2 requested a review from janhack January 31, 2023 11:07
@shuse2 shuse2 enabled auto-merge (squash) January 31, 2023 11:37
@shuse2 shuse2 merged commit 98e4765 into development Jan 31, 2023
@shuse2 shuse2 deleted the 8086-fix_init_signing branch January 31, 2023 12:24
This was referenced Feb 13, 2023
Closed
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ishantiw ishantiw ishantiw approved these changes

@janhack janhack janhack approved these changes

Assignees

@shuse2 shuse2

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fail to restart node
3 participants
@shuse2 @ishantiw @janhack